feat: replace legacyInstamine option with `instamine=eager|strict` #2013

davidmurdoch · 2022-01-07T20:34:35Z

No description provided.

davidmurdoch · 2022-01-07T20:41:05Z

src/chains/ethereum/ethereum/src/blockchain.ts

+        // ```
+        // If we don't have this delay here the messages will be sent before
+        // the call has a chance to listen to the event.
+        setImmediate(async () => {


It turns out this was broken in greedy mode!

what is broken?

The delay wasn't working. Our existing tests caught it when I switched the default.

await provider.send("eth_sendTransaction", ...); await provider.once("message"); // <- this wasn't work, but it should.

davidmurdoch · 2022-01-07T20:42:19Z

src/chains/ethereum/ethereum/src/miner/miner.ts

-        // first mine operation to be fully complete.
-        await emitBlockProm;
-      }
+      this.emit("block", finalizedBlockData);


We don't need to wait because evm_mine and miner_start wait on their own.

If you feel it necessary to explain this in the review it likely belongs as a comment in the code.

It explains why the old code was removed.

I'm actually having to revisit this because I noticed if someone sends stops the miner via miner_stop, send enough transactions to fill two blocks, then calls miner_start, all pending transactions aren't mined before miner_start returns... which is what we want.

I think evm_mine suffers from a similar issue, but I haven't yet figured out what yet.

These issues might be rare enough that I can wait until after v7 stable to fix them, as it might take a bit of refactoring to get the timings correct.

davidmurdoch · 2022-01-07T20:53:17Z

src/chains/ethereum/ethereum/tests/api/evm/snapshot.test.ts

+        const [finalLaterTxsReceipts, finalLaterTxsTransactions] =
+          await Promise.all([
+            finalLaterTxsReceiptsProm,
+            finalLaterTxsTransactionsProm
+          ]);


(prettier change)

davidmurdoch · 2022-01-07T20:53:25Z

src/chains/ethereum/ethereum/tests/api/miner/miner.test.ts

+        const { status, blockNumber } = await provider.send(
+          "eth_getTransactionReceipt",
+          [txHash]
+        );


(prettier change)

davidmurdoch · 2022-01-07T20:53:34Z

src/chains/ethereum/ethereum/tests/api/eth/subscribe.test.ts

@@ -75,8 +75,7 @@ describe("api", () => {
                extraData: "0x",
                gasLimit: gasLimit,
                gasUsed: "0x0",
-                hash:
-                  "0xe2c5d64b9e17e25abc0589c378b77adecf06668dd3c073ab9c53dec51baf2048",
+                hash: "0xe2c5d64b9e17e25abc0589c378b77adecf06668dd3c073ab9c53dec51baf2048",


(prettier change)

davidmurdoch · 2022-01-07T20:53:45Z

src/chains/ethereum/ethereum/tests/api/eth/getTransactionReceipt.test.ts

+          const result = await strictInstamineProvider.send(
+            "eth_getTransactionReceipt",
+            [hash]
+          );


(prettier change)

davidmurdoch · 2022-01-07T20:54:15Z

src/chains/ethereum/ethereum/src/blockchain.ts

+    const { gas, structLogs, returnValue, storage } =
+      await this.#traceTransaction(
+        newBlock.transactions[transaction.index.toNumber()],
+        trie,
+        newBlock,
+        options
+      );


(prettier change)

davidmurdoch · 2022-01-07T20:54:24Z

src/chains/ethereum/ethereum/src/blockchain.ts

+          blocks.earliest = blocks.latest =
+            await this.#blockBeingSavedPromise.then(({ block }) => block);


(prettier change)

src/chains/ethereum/ethereum/src/blockchain.ts

src/chains/ethereum/ethereum/src/api.ts

cds-amal

Left comments as questions mostly. Not a fan of greedy and strict.

src/chains/ethereum/ethereum/src/api.ts

cds-amal · 2022-01-07T21:23:54Z

src/chains/ethereum/ethereum/src/miner/miner.ts

-        // first mine operation to be fully complete.
-        await emitBlockProm;
-      }
+      this.emit("block", finalizedBlockData);


If you feel it necessary to explain this in the review it likely belongs as a comment in the code.

src/chains/ethereum/ethereum/tests/helpers/getProvider.ts

MicaiahReid

One more change request I couldn't make in the code - can we rename greedyInstamining.test.ts since we're not calling it that anymore?

src/chains/ethereum/ethereum/src/api.ts

src/chains/ethereum/ethereum/src/blockchain.ts

src/chains/ethereum/ethereum/tests/api/eth/getTransactionReceipt.test.ts

MicaiahReid · 2022-01-10T17:11:28Z

src/chains/ethereum/ethereum/tests/helpers/getProvider.ts

  options.logging = options.logging || { logger: { log: () => {} } };

  // set `asyncRequestProcessing` to `true` by default
  let doAsync = options.chain.asyncRequestProcessing;
  doAsync = options.chain.asyncRequestProcessing =
    doAsync != null ? doAsync : true;

-  // don't write to stdout in tests
-  if (!options.logging.logger) {


Why are we removing this?

It didn't appear to actually do anything in our tests (I replaced it with throw new Error... and all tests still passed)

MicaiahReid · 2022-01-10T17:12:11Z

src/chains/ethereum/options/src/miner-options.ts

-     * the transaction's hash is returned to the caller. If `legacyInstamine` is
-     * `true`, `blockTime` must be `0` (default).
+     * Set the instamine mode to either "eager" (default) or "strict".
+     *  * In "eager" mode a transaction will be included in a block before the


Suggested change

* * In "eager" mode a transaction will be included in a block before the

* * In "eager" mode a transaction will be included in a block before

MicaiahReid · 2022-01-10T17:13:06Z

src/chains/ethereum/options/src/miner-options.ts

     *
-     * @defaultValue false
-     * @deprecated Will be removed in v4
+     * @defaultValue eager


Suggested change

* @defaultValue eager

* @defaultValue "eager"

MicaiahReid · 2022-01-10T17:14:12Z

src/chains/ethereum/options/src/miner-options.ts

-    legacyName: "legacyInstamine",
-    cliType: "boolean"
+    cliDescription: `Set the instamine mode to either "eager" (default) or "strict".
+ * In "eager" mode a transaction will be included in a block before the its hash is returned to the caller.


Suggested change

* In "eager" mode a transaction will be included in a block before the its hash is returned to the caller.

* In "eager" mode a transaction will be included in a block before its hash is returned to the caller.

…pt.test.ts Co-authored-by: Micaiah Reid <[email protected]>

…amine

feat: replace legacyInstamine option with instamine=greedy|strict

35be9aa

davidmurdoch mentioned this pull request Jan 7, 2022

fix: force legacyInstamine for transactions sent over HTTP #1998

Closed

davidmurdoch requested a review from MicaiahReid January 7, 2022 20:36

davidmurdoch added 7 commits January 7, 2022 15:38

refactor evm_mine

f81314a

remove console.logs

864a882

clarify comment

ead0996

restore this file (only had prettier changes)

3bce76d

remove prettier only changes

7d6438a

remove unused code

1f555a2

remove unused var

17be732

davidmurdoch commented Jan 7, 2022

View reviewed changes

cds-amal reviewed Jan 7, 2022

View reviewed changes

src/chains/ethereum/ethereum/src/api.ts Outdated Show resolved Hide resolved

cds-amal reviewed Jan 7, 2022

View reviewed changes

remove dead code, thanks amal!

e355e5d

davidmurdoch changed the title ~~feat: replace legacyInstamine option with `instamine=greedy|strict~~ feat: replace legacyInstamine option with instamine=greedy|strict Jan 7, 2022

change greedy to eager

9e33e4a

davidmurdoch changed the title ~~feat: replace legacyInstamine option with instamine=greedy|strict~~ feat: replace legacyInstamine option with instamine=eager|strict Jan 7, 2022

MicaiahReid suggested changes Jan 10, 2022

View reviewed changes

davidmurdoch and others added 6 commits January 10, 2022 12:22

remove incorrect warning

337e637

fix code comment

1085ddc

Update src/chains/ethereum/ethereum/tests/api/eth/getTransactionRecei…

e5d966e

…pt.test.ts Co-authored-by: Micaiah Reid <[email protected]>

fix typos

f0c26ff

Merge branch 'instamine' of github.com:trufflesuite/ganache into inst…

429d3ad

…amine

i'm not broken, you're broken

50ca402

davidmurdoch merged commit c32aee8 into develop Jan 11, 2022

davidmurdoch deleted the instamine branch January 11, 2022 17:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: replace legacyInstamine option with `instamine=eager|strict` #2013

feat: replace legacyInstamine option with `instamine=eager|strict` #2013

davidmurdoch commented Jan 7, 2022

davidmurdoch Jan 7, 2022

cds-amal Jan 7, 2022

davidmurdoch Jan 7, 2022 •

edited

Loading

davidmurdoch Jan 7, 2022

cds-amal Jan 7, 2022

davidmurdoch Jan 7, 2022

davidmurdoch Jan 8, 2022

davidmurdoch Jan 7, 2022

davidmurdoch Jan 7, 2022

davidmurdoch Jan 7, 2022

davidmurdoch Jan 7, 2022

davidmurdoch Jan 7, 2022

davidmurdoch Jan 7, 2022

cds-amal left a comment

cds-amal Jan 7, 2022

MicaiahReid left a comment

MicaiahReid Jan 10, 2022

davidmurdoch Jan 10, 2022

MicaiahReid Jan 10, 2022

MicaiahReid Jan 10, 2022

MicaiahReid Jan 10, 2022

		blocks.earliest = blocks.latest =
		await this.#blockBeingSavedPromise.then(({ block }) => block);

	* * In "eager" mode a transaction will be included in a block before the
	* * In "eager" mode a transaction will be included in a block before

	* In "eager" mode a transaction will be included in a block before the its hash is returned to the caller.
	* In "eager" mode a transaction will be included in a block before its hash is returned to the caller.

feat: replace legacyInstamine option with instamine=eager|strict #2013

feat: replace legacyInstamine option with instamine=eager|strict #2013

Conversation

davidmurdoch commented Jan 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidmurdoch Jan 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cds-amal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MicaiahReid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat: replace legacyInstamine option with `instamine=eager|strict` #2013

feat: replace legacyInstamine option with `instamine=eager|strict` #2013

davidmurdoch Jan 7, 2022 •

edited

Loading